Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Allow to customize GridToolbarExport's CSV export #1695

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

michallukowski
Copy link
Contributor

@michallukowski michallukowski commented May 18, 2021

This feature allows to:
- set the file name
- set flag utf8WithBom to generate csv file as UTF-8 with BOM

It may be extended with further options in the future

Example how to use:

import { GridToolbarExport, GridExportCsvOptions } from '@material-ui/data-grid';

<GridToolbarExport
  csvOptions={{
    fileName: 'csvWithBom',
    utf8WithBom: true,
  } as GridExportCsvOptions}
/>

It generate file "csvWithBom.csv" with sequence of bytes (0xEF, 0xBB, 0xBF) at the beginning.

One iteration on #1440

@m4theushw m4theushw changed the title [DataGrid] Add optional exportConfiguration props to GridToolbarExport issue:#1440 [DataGrid] Add an exportConfiguration prop to GridToolbarExport May 19, 2021
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label May 20, 2021
@@ -12,7 +12,7 @@ import { GridExportFormat } from '../models/gridExport';
*/
export function exportAs(
blob: Blob,
extension: GridExportFormat = 'csv',
extension: GridExportFormatExtension = 'csv',
filename: string = document.title,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the default filename to include the date.
something like export-21.05.2021-19:21.csv?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtassone This sounds off-topic to the PR, but we could create a new issue for it 👍

@oliviertassinari oliviertassinari added the new feature New feature or request label May 20, 2021
@oliviertassinari oliviertassinari self-assigned this May 31, 2021
…mui#1440

This feature allows to:
- set the file name
- set flag utf8WithBom to generate csv file as UTF-8 with BOM

It may be extended with further options in the future
@oliviertassinari oliviertassinari force-pushed the my-grid-toolbar-export-options branch 7 times, most recently from d4c40b4 to 16cb13b Compare May 31, 2021 16:58
@oliviertassinari oliviertassinari removed their assignment May 31, 2021
@oliviertassinari oliviertassinari dismissed their stale review May 31, 2021 17:02

I have pushed changes to match what the overall feedback direction seems to be

@oliviertassinari oliviertassinari changed the title [DataGrid] Add an exportConfiguration prop to GridToolbarExport [DataGrid] Allow to customize GridToolbarExport's CSV export May 31, 2021
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
export type GridExportFormat = 'csv';
export interface GridExportCsvOptions {
fileName?: string;
utf8WithBom?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the withBom suffix? I don't think it brings any value

Copy link
Member

@oliviertassinari oliviertassinari Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtassone How will a developer differentiate if he's going to include a BOM or not?

Copy link
Member

@dtassone dtassone Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what they want is UTF8, the BOM part is just for the reader...

Copy link
Member

@oliviertassinari oliviertassinari Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the default format? UTF8? Are developers using any other format?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure actually, I thought it was ASCII and we were missing the special characters.

Copy link
Member

@oliviertassinari oliviertassinari Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michallukowski How about we start without, only an option to customize the filename?

Suggested change
utf8WithBom?: boolean;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for this pull request was to add an option for developers to set a flag if they wanted to add a BOM for a utf-8 encoded file as mentioned in #1440. The filename setting came up by the way. What's the problem with the utf8WithBom option? If you really don't want this option, we can remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can default to utf8 with Bom, and either remove or reverse the prop so only users that don't want BOM for a special reason would actually set it.

Copy link
Contributor Author

@michallukowski michallukowski Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to add a BOM by default. According to the Unicode Standard:

Use of a BOM is neither required nor recommended for UTF-8, but may be encountered in contexts where UTF-8 data is converted from other encoding forms that use a BOM or where the BOM is used as a UTF-8 signature

https://en.wikipedia.org/wiki/Byte_order_mark

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context: ag-grid/ag-grid#3916

formatOptions?: GridExportCsvOptions;
}

type GridExportFormatOption = GridExportFormatCsv;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these 2 types, why not only 1?
Not sure everybody would agree but I would export the type used in the props, so users can easily type the component props.

Copy link
Member

@oliviertassinari oliviertassinari Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not only 1?

I assume that because as soon as we have excel, it needs:

Suggested change
type GridExportFormatOption = GridExportFormatCsv;
type GridExportFormatOption = GridExportFormatCsv | GridExportFormatExcel;

Not sure everybody would agree but I would export the type used in the props, so users can easily type the component props.

Why would a developer ever care about them? Isn't this type completely internal to the component, with no public API implications? I mean, it seems completely dependent on how GridToolbarExport is written.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you export the button to recompose your toolbar and you allow to pass props in the button, you might want to type those props and what's inside them

Copy link
Member

@oliviertassinari oliviertassinari Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you export the button to recompose your toolbar and you allow to pass props in the button, you might want to type those props and what's inside them

Then I don't understand your previous comment. The GridToolbarExportProps type is already public for this use case.

You can use https://codesandbox.io/s/material-demo-forked-o7cee?file=/demo.tsx to provide an example if you were thinking of something else, it uses the last commit of this PR.

@m4theushw m4theushw merged commit e9465da into mui:master Jun 7, 2021
@michallukowski michallukowski deleted the my-grid-toolbar-export-options branch June 7, 2021 13:37
@oliviertassinari
Copy link
Member

The PR wasn't rebased, it broke master, I pushed a new commit, fixed:

Capture d’écran 2021-06-07 à 22 16 09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants